Add signal handling to stream manager #2950
Conversation
…the stream manager
| return 0; | ||
| } | ||
|
|
||
| int EventLoopImpl::unRegisterSignal(int sig) { |
There was a problem hiding this comment.
where is this function hooked up?
There was a problem hiding this comment.
This function is not actually used yet. I included it, because other events also have a unregister function and it might be useful sometime in the future.
I can also remove it.
There was a problem hiding this comment.
kk. It is ok to leave it there to me. Let's see if @srkukarni has any input.
| EventLoopImpl ss; | ||
|
|
||
| void sigtermHandler(int signum) { | ||
| ss.loopExit(); |
There was a problem hiding this comment.
I am a bit concerned that if loopExit() is safe to call here.
- which thread it is running on and if there might be racing condition.
- if it is possible that loopExit() might get stuck.
There was a problem hiding this comment.
I think the sigtermHandler should run the in the same thread as any other callback function and loopExit() directly calls event_base_loopbreak() which I think should be safe to call. Here in the libevent book they call event_base_loopbreak() in a very similar way.
But maybe someone with more experience with libevent could give some input on that.
There was a problem hiding this comment.
@nwangtw - stmgr runs in a single thread. hence loopExit is fine in the context of sigterm handler.
There was a problem hiding this comment.
@nwangtw - stmgr runs in a single thread. hence loopExit is fine in the context of sigterm handler.
| if (event_del(mSignalEvents[sig]->event()) != 0) { | ||
| throw heron::error::Error_Exception(errno); | ||
| } | ||
| delete mSignalEvents[sig]; |
There was a problem hiding this comment.
safer way is to use a temp var to make sure the object is erased from the map before deleting it.
There was a problem hiding this comment.
I noticed that this is done the same way multiple times in event_loop_impl.cpp (See e.g. unRegisterForRead or unRegisterForWrite).
Should I change it for every unRegsiter* function or should I leave it that way. It would be inconsistent if I would only change the unRegisterSignal.
There was a problem hiding this comment.
@Glorfischi - can you please do those changes for other unRegister events as well?
|
@nwangtw @Glorfischi - is this ready to be merged? |
|
@nwangtw - can you please check if this is ok to merge it? @Glorfischi - have you fixed all the feedback. |
|
Sorry for the delay. I have a fix ready for one of the feedback, but I'll wait for a response before I push it. See the comment on the As for the |
…the event is erased from the map before deleting it.
| if (event_del(mSignalEvents[sig]->event()) != 0) { | ||
| throw heron::error::Error_Exception(errno); | ||
| } | ||
| auto* event = mSignalEvents[sig]; |
There was a problem hiding this comment.
do you need an auto* ? just auto event will be fine, isn't?
There was a problem hiding this comment.
oh yes I think you're right. That * is useless.
| @@ -136,8 +176,9 @@ int EventLoopImpl::unRegisterForRead(int fd) { | |||
| // cout << "event_del failed for fd " << fd; | |||
| throw heron::error::Error_Exception(errno); | |||
| } | |||
| delete mReadEvents[fd]; | |||
| auto* event = mReadEvents[fd]; | |||
There was a problem hiding this comment.
same here and other places.
This adds signal handling to the
EventLoopImplin order to add a SIGTERM handler to the stream manager. (See #2947)I tried to handle signal events similarly to the other events. This resulted in quite a lot of boilerplate code, I am not completely sure this is the right way to go.